refactor: harden exec failure wrapping end to end#1074
Merged
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
Member
Author
|
CI needs attention before review: Reported gate failures:
Please run |
Follow-up to #1072, closing the structural gaps that let the missing-processExitError bug class exist: - requireExecSuccess(result, message, extra?) in utils/exec.ts: guards an allowFailure result and throws the curated COMMAND_FAILED (flag set via execFailureDetails) itself. Result-side by design — tool providers and executor overrides return results without throwing, so an ExecOptions knob interpreted at spawn time would silently not fire on those paths. ~30 pure guard-and-throw sites across apple/android/web converted; sites with tolerance branches, cleanup, or exit-0 reachability keep their explicit shape. - Source-scan guard (src/__tests__/exec-wrap-guard.test.ts): fails when an AppError('COMMAND_FAILED', ...) details literal rebuilds the stdout/stderr/exitCode trio inline without the helper; intentional holdouts opt out with a documented exec-guard-allow comment. The guard immediately found three wrap sites the July audit missed (runtime-hints run-as probe, device-ready not_ready branch, perf export table) — now converted. - coerceExecResult at the provider boundaries (executor overrides, apple tool provider scope, android adb provider scope): SDK-supplied callbacks cross an unchecked boundary; coercing once there replaces the per-site String(result.stdout ?? '') defensiveness, which is removed. - normalizeError stderr excerpts now strip noise prefixes (adb:/xcrun:/ simctl:/error:) before rendering; skip/strip lists stay in the kernel deliberately (platforms cannot hook normalizeError) with a comment marking the registry escape hatch if they grow. - AppErrorDetails exported and documented in kernel/errors.ts: the magic keys (hint, processExitError, retriable, reason, diagnosticId, logPath, stdout/stderr/exitCode) now carry types and doc comments. - runIosDevicectl gains tolerateOutput; the devicectl uninstall path in app-install.ts reuses it instead of duplicating the wrap + hint logic (its failure hint now falls back to the devicectl default hint).
Main landed the central adb failure classifier (androidAdbResultError +
withAdbFailureHintProvider) while this branch was in flight. Resolution:
- Android call sites keep main's androidAdbResultError form — it composes
execFailureDetails with the classified adb hint, which is strictly
richer than a plain requireExecSuccess conversion for adb invocations.
requireExecSuccess remains the shape for non-adb tools and apple/web.
- Provider result coercion folds into withAdbFailureHintProvider's
enrichment pass (exec/pull/install), so the existing WeakSet memo also
prevents coercer stacking.
- The reverse-remove wrap in adb-executor now uses androidAdbResultError
instead of hand-rolling the trio (flagged by the exec-wrap guard).
- Guard-test scan split into helpers (fallow cyclomatic threshold) and
the perf artifact-tail clone carries fallow-ignore markers on both
platforms, matching the pre-existing marker on the apple side.
- Two expectations updated for the stderr noise-prefix strip: classifier
compose test and perf sampling reason ('device offline', no 'error:').
a4f0b0e to
5b66fed
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Follow-up to #1072 addressing the structural issues that let the missing-
processExitErrorbug class exist, in one pass:1.
requireExecSuccesscombinator (+ ~30 site conversions)requireExecSuccess(result, message, extra?)insrc/utils/exec.tsguards anallowFailureresult and throws the curatedCOMMAND_FAILED(flag set viaexecFailureDetails) itself. Pure guard-and-throw sites collapse from five lines to one expression, and forgetting the flag becomes impossible.Design note: this is deliberately a result-side combinator, not an
ExecOptions.failureknob. Tool providers, adb providers, andCommandExecutorOverrideare SDK-supplied callbacks that return results without throwing — an option interpreted at spawn time would silently not fire on those paths. The combinator behaves identically under every executor.extraaccepts a function so failure-only work (hint classification) is not paid on success.Sites with tolerance branches ('found nothing to terminate',
isMissingAppErrorOutput, 'already booted'), pre-throw cleanup, or exit-0 reachability keep their explicit shape.2. Self-enforcing guard
src/__tests__/exec-wrap-guard.test.tsscanssrc/and fails when anAppError('COMMAND_FAILED', ...)details literal rebuilds thestdout/stderr/exitCodetrio inline (longhand or shorthand) without the helper. Intentional holdouts opt out with// exec-guard-allow: <reason>— every marker documents why that site stays unflagged. The guard immediately proved its worth by finding three wrap sites the July audit missed (daemon/runtime-hints.tsrun-as probe,daemon/device-ready.tsnot_ready branch,perf.tsexport table) — all converted here.3. Provider-boundary result coercion
coerceExecResultnormalizesstdout/stderrto strings (and non-numberexitCodeto 1 — the same failure branch such a result already landed in at every guard) once at the three boundaries where SDK-supplied callbacks hand results back: executor overrides inrunCmd/runCmdStreaming, the apple tool-provider scope, and the android adb-provider scope. The per-siteString(result.stdout ?? '')defensiveness in devicectl/simulator/device-ready/target-shutdown/plist is removed — downstream code can now trust the types.4. Kernel: stderr-excerpt noise stripping + documented details contract
firstStderrLinestripsadb:/xcrun:/simctl:/error:prefixes before rendering, so agents readdevice offlineinstead ofadb: error: device offline. The skip/strip lists stay in the kernel deliberately (platforms cannot hooknormalizeError); a comment marks the registry escape hatch if the lists grow.AppErrorDetailsis now exported and documented — the magic keys (hint,processExitError,retriable,reason,diagnosticId,logPath,stdout/stderr/exitCode) carry types and doc comments.exitCodeadmitsnullto mirror raw child-process exit events (signal kills).5. devicectl wrap dedupe
runIosDevicectlgainstolerateOutput; the devicectl uninstall path inapp-install.tsreuses it instead of duplicating the whole wrap + hint resolution. Small behavior note: its failure hint now falls back to the devicectl default hint instead of no hint.Behavior changes a reviewer should know
reasonis nowdevice offline(prefix stripped) instead oferror: device offline.requireExecSuccess/execFailureDetailssites that previously omittedstdout/exitCodefrom details now include them.undefinedinto details/messages; non-number exitCodes coerce to 1 (previously they already fell into every failure branch, but withundefinedin details).Testing
pnpm check:quickclean,pnpm formatapplied.requireExecSuccesssuccess/failure/lazy-extras,coerceExecResultidentity + repair, three stderr noise-prefix cases inerrors.test.ts.Relates to ADR 0010 (#1071) section 4 — this PR strengthens "prefer exec.ts errors as-is" by making the curated-message case expressible without hand-rolling.